Nexus fix: OwnableValidator & OwnableExecutor integration + tests, + add missing features after viem refactor + fix issues & conflicts +++ #576

VGabriel45 · 2024-09-17T08:38:26Z

PR-Codex overview

This PR focuses on refactoring the handling of signer instead of holder in various modules and updates the related tests. It also introduces new functionalities and makes structural adjustments to improve the codebase.

Detailed summary

Replaced holder with signer in multiple files.
Updated toHolder utility to toSigner.
Modified function signatures and types to use Signer.
Added new module getPreviousModule for retrieving previous module addresses.
Enhanced tests for OwnableExecutor and OwnableValidator.
Adjusted module installation and uninstalling logic.
Updated the package.json dependencies.
Changed addresses in addresses.ts.

The following files were skipped due to too many changes: src/sdk/modules/validators/ownableValidator.test.ts, src/sdk/modules/validators/OwnableValidator.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

livingrockrises · 2024-09-17T09:48:11Z

conflicts need to be resolved.

src/__contracts/addresses.ts

src/account/NexusSmartAccount.ts

src/account/utils/Types.ts

src/modules/utils/Helper.ts

livingrockrises · 2024-09-17T09:57:01Z

src/modules/validators/OwnableValidator.ts

+import type { Module } from "../utils/Types.js"
+
+export class OwnableValidator extends BaseValidationModule {
+    public smartAccount: NexusSmartAccount


is it useful for all modules' instances to get SA instance passed?

Yes, since we are building user operations for module transactions

ok, my only concern was regarding applying defaultValidatorModule when first time SA instance is created and module will expect SA instance passed.

livingrockrises · 2024-09-17T09:57:27Z

src/modules/validators/OwnableValidator.ts

+            parseAbiParameters("uint256 threshold, address[] owners"),
+            moduleConfig.data
+        )
+        super(moduleConfig, smartAccount.getSmartAccountOwner())


isn't super supposed to accept a signer?

private constructor(moduleConfig: Module, signer: SmartAccountSigner) { super(moduleConfig, signer) }

That's what smartAccount.getSmartAccountOwner() is passing

ok I thought by name it returns an address and not signer object.

livingrockrises · 2024-09-17T09:58:23Z

src/modules/validators/OwnableValidator.ts

+    public threshold: number
+
+    private constructor(
+        moduleConfig: Module,


Module has become a multi-purpose type. have some questions around it, as data can be interpreted multiple ways when it comes to installations and uninstallation if I'm not wrong?

You propose to create specific types for each module ? Like instead of having "Module" we would have "OwnableValidatorModuleConfig" ?

I think it is fine for now if only common properties are mandatory

src/modules/validators/OwnableValidator.ts

livingrockrises · 2024-09-17T10:00:00Z

src/modules/validators/OwnableValidator.ts

+        smartAccount: NexusSmartAccount,
+        owners: Address[],
+        threshold?: number,
+        hook?: Address


I wonder what is the purpose of this. have noticed in module-sdk as well where a hook is always tied to any kind of a module..

This is for Kernel modules, Kernel has a hook that is associated with every validator, executor and fallback

src/modules/validators/OwnableValidator.ts

livingrockrises · 2024-09-17T10:01:20Z

src/modules/validators/OwnableValidator.ts

+        hook?: Address
+    ): Promise<OwnableValidator> {
+        if (
+            !owners.includes(await smartAccount.getSmartAccountOwner().getAddress())


we're assuming here already initialised smart account. MUST be initialised with K1 as default

but what if I had passed defaultValidationModule as PasskeyValidator instance in the future? generally how does smartAccount.getSmartAccountOwner() give you answer and fit with .getAddress() later.
just food for thought

I don't get this, need more context

specific to nexus account. if default validator module is different then this would not have expected result

livingrockrises · 2024-09-17T10:03:02Z

src/modules/validators/OwnableValidator.ts

+        if (
+            !owners.includes(await smartAccount.getSmartAccountOwner().getAddress())
+        ) {
+            throw Error("Signer needs to be one of the owners")


you mean one of the owners should also be an onwer through K1 validator module assumed to be installed.

No, my initial thought was that we need the smart account owner eoa to be part of this owners array too but it does not, I did some changes to fix this.

yup. could be totally unrelated owners

livingrockrises · 2024-09-17T10:04:16Z

src/modules/validators/OwnableValidator.ts

+            data: calldata,
+            value: 0n
+        }
+        const userOp = await this.smartAccount.buildUserOp([transaction]);


I see benefit of passing smartAccount instance is userop can be sent right away. in stead of just requesting for calldata

Exactly, we add a new layer of utility to the module instance.

src/modules/validators/OwnableValidator.ts

tests/modules.ownableValidator.write.test.ts

livingrockrises

review i

livingrockrises · 2024-09-17T10:11:32Z

conflicts to be resolved

…eployed accounts

…-sdk into nexus_fix/ownableValidator

github-actions · 2024-09-19T12:58:15Z

size-limit report 📦

Path	Size
core (esm)	11.27 KB (+1.46% 🔺)
core (cjs)	17.34 KB (+1.16% 🔺)
bundler (tree-shaking)	197 B (0%)
paymaster (tree-shaking)	108 B (0%)

livingrockrises · 2024-09-19T14:10:41Z

ok new conflicts here

…-sdk into nexus_fix/ownableValidator

joepegler

Great to have install functionality back. Thanks for bringing it back 💪

src/sdk/account/toNexusAccount.test.ts

src/sdk/account/toNexusAccount.ts

src/sdk/modules/validators/ownableValidator.test.ts

src/test/callDatas.js.map

src/test/testUtils.ts

joepegler · 2024-09-20T21:13:25Z

Just a wider point on our modules. I don't like the folder structure any more. It's difficult to know where to look or why the folders are arranged in the way that they are. I also don't like the Base class stuff that we have now. It feels dated, and I'm not sure it even makes sense given how much variety we will have from our modules. I prefer rhinestone flatter / simplified way of organising modules. Can we replicate something similar to what they have done, paired with decorators that can be extended by a nexusClient instead? I realise this might require a different PR

joepegler · 2024-09-20T21:18:12Z

I've not given this more that a moments thought, but something like...

to

modules >
  smartSessions >
    abi
    constants
    installation
    index
    usage
    decorators
  ownables > 
    abi
    constants
    installation
    index
    usage
    decorators

livingrockrises · 2024-09-22T10:01:23Z

checks are failing.

livingrockrises · 2024-09-22T10:03:18Z

I've not given this more that a moments thought, but something like...

`to`

modules >
  smartSessions >
    abi
    constants
    installation
    index
    usage
    decorators
  ownables > 
    abi
    constants
    installation
    index
    usage
    decorators

folder for each module is ok. most of them will be validators though for which we precisely need these classes because of default and active validator concept.

src/sdk/clients/decorators/erc7579/index.ts

src/sdk/clients/decorators/erc7579/uninstallModule.ts

VGabriel45 · 2024-09-23T16:27:56Z

getActiveValidationModule

We don't need this, instead we will be migrating to use rhinestone's sdk for module.

livingrockrises · 2024-09-23T18:31:58Z

src/sdk/account/toNexusAccount.ts

-  const defaultedActiveModule =
-    activeModule ??
+  let defaultedActiveModule =
+    activeValidationModule ??


we can also call this activeValidator
but can keep this as note for now and still merge PR.

livingrockrises

leaving approved

VGabriel45 added 2 commits September 17, 2024 11:27

fixed ownable validator implementation and tests

6cf5f63

refactor: refactor OwnableValidator instance class

f358547

VGabriel45 requested review from joepegler and livingrockrises September 17, 2024 09:26

feat: added signature override to sendTransaction

e17587a